Skip to content

Fix CnsNodeVMBatchAttachment API review to follow better conventions #3625

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix CnsNodeVMBatchAttachment API review to follow better conventions #3625

wants to merge 1 commit into from

Conversation

skogta
Copy link
Contributor

@skogta skogta commented Sep 30, 2025 •
edited
Loading

What this PR does / why we need it:

This PR improves the CnsNodeVMBatchAttachment API. There are the changes:

  1. Change name of the API to CnsNodeVMBatchAttachment from CnsNodeVmBatchAttachment.
  2. Use Enum validation for SharingMode and DiskMode.
  3. Change nodeuuid to nodeUUID and diskuuid to diskUUID.
  4. Add +optional and +required fields wherever possible.
  5. Fix typo - shraring to sharing.
  6. Add shortname for the API.
  7. Add an additonal printer column for the API.

Testing done:

K8s short hand and printer column are working as expected:

root@420fe5a752cfe1034e7f80d586f417a8 [ ~ ]# k get batchattach -n test
NAME         NODEUUID
test-new-2   424ea6db-49bd-4ba8-bd6b-676353617e3c

Was able to attach the volume with the new Batch attach API

root@420fe5a752cfe1034e7f80d586f417a8 [ ~ ]# k describe cnsnodevmbatchattachment.cns.vmware.com test-new-2 -n test
Name:         test-new-2
Namespace:    test
Labels:       <none>
Annotations:  <none>
API Version:  cns.vmware.com/v1alpha1
Kind:         CnsNodeVMBatchAttachment
Metadata:
  Creation Timestamp:  2025-09-30T08:35:56Z
  Finalizers:
    cns.vmware.com
  Generation:        1
  Resource Version:  5020275
  UID:               e0ecc151-7657-4d24-b815-7429ca0269f7
Spec:
  Node UUID:  424ea6db-49bd-4ba8-bd6b-676353617e3c
  Volumes:
    Name:  disk-1
    Persistent Volume Claim:
      Claim Name:  pvc-rwo-2
    Name:          disk-2
    Persistent Volume Claim:
      Claim Name:      pvc-rwx-2
      Controller Key:  1002
      Disk Mode:       independent_persistent
      Sharing Mode:    sharingMultiWriter
      Unit Number:     6
Status:
  Volumes:
    Name:  disk-1
    Persistent Volume Claim:
      Disk UUID:      6000C29e-70c5-0d8d-34b6-de8a503b8f1d
      Attached:       true
      Claim Name:     pvc-rwo-2
      Cns Volume Id:  b882f547-a587-40a4-8aa5-5c44210148c1
    Name:             disk-2
    Persistent Volume Claim:
      Disk UUID:      6000C298-07f8-d142-c063-408351683b00
      Attached:       true
      Claim Name:     pvc-rwx-2
      Cns Volume Id:  765ccff6-bd82-4aae-9fc7-5fa1edf8b86e
Events:
  Type    Reason                      Age   From            Message
  ----    ------                      ----  ----            -------
  Normal  NodeVmBatchAttachSucceeded  12s   cns.vmware.com  ReconcileCnsNodeVMBatchAttachment: Successfully processed instance test/test-new-2 in namespace "test/test-new-2".

WCP precheckin: https://jenkins-vcf-csifvt.devops.broadcom.net/job/wcp-instapp-e2e-pre-checkin/442/
VKS precheckin: https://jenkins-vcf-csifvt.devops.broadcom.net/view/instapp/job/vks-instapp-e2e-pre-checkin/465/

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 30, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: skogta
Once this PR has been reviewed and has the lgtm label, please assign raunakshah for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 30, 2025
@k8s-ci-robot
Copy link
Contributor

Hi @skogta. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 30, 2025
@skogta skogta force-pushed the topic/skogta/apiFixes branch 2 times, most recently from d0157b6 to e0c70d2 Compare September 30, 2025 08:02
@skogta skogta changed the title Fix API review comments for CnsNodeVMBatachAttachment API Fix CnsNodeVMBatachAttachment API review to follow better conventions Sep 30, 2025
aruneshpa
aruneshpa reviewed Sep 30, 2025
View reviewed changes
Copy link

@aruneshpa aruneshpa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @skogta for addressing the review comments.

@divyenpatel
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 30, 2025
@skogta skogta force-pushed the topic/skogta/apiFixes branch 2 times, most recently from c2c1302 to 2e54a60 Compare October 4, 2025 09:47
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 4, 2025
@skogta skogta force-pushed the topic/skogta/apiFixes branch from 2e54a60 to 510b77d Compare October 4, 2025 09:50
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 8, 2025
@skogta skogta force-pushed the topic/skogta/apiFixes branch from 510b77d to ba024ed Compare October 8, 2025 06:35
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 8, 2025
@skogta skogta changed the title Fix CnsNodeVMBatachAttachment API review to follow better conventions Fix CnsNodeVMBatchAttachment API review to follow better conventions Oct 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chethanv28 chethanv28 Awaiting requested review from chethanv28

@deepakkinni deepakkinni Awaiting requested review from deepakkinni

1 more reviewer

@aruneshpa aruneshpa aruneshpa left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@skogta @k8s-ci-robot @divyenpatel @aruneshpa